-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix concurrent R/W access to link properties #87
Conversation
To understand the issue better, could you explain when and where link properties can be changed concurrently? Ideally, the |
@mickael-menu The example in the stack trace with the archive fetcher is technically solvable, you're correct, however, programs using the go-toolkit may want to modify the properties of the link after receiving them from the toolkit, for example, to add custom metadata to the link that was computed by their code. In this case, if the properties were immutable, they would have to create a new instance of the struct/property. |
I think this discussion goes further than this particular PR, so I opened a new discussion. This should already answer some of your concerns regarding JSON marshalling and struct property accessors. If it makes sense and you agree with my point, then for this data race issue we "just" need to refactor to not mutate the structs in the archive fetcher. In future PRs we could migrate to immutable list/map types, or not. If you disagree with my take, let's talk about it on the discussion thread and during meetings. |
@mickael-menu I'm working on getting the go-toolkit in a state for building and testing, and I'm coming back to getting this PR merged . While I think going with the immutability strategy is a good idea in the long-term, I actually realized it's not super easy to make the My overall question is, given the goals of not spending significant on a refactor of this (so the branch can be merged), and not incurring additional memory usage by e.g. making a complete copy of the Link to prevent mutations, what do you think is the best way to move forwards with this? Keeping this as it is for the moment and then going back to it later is also an option of course, while it's not ideal, the current implementation "works". |
I'm concerned that if we don't get this right in this PR, the technical debt will continue to accumulate and we may never take the time to address this issue. Implementers could also rely on the mutability of the link properties. You made a good point about the Kotlin changes and this could actually serve as inspiration for the solution. How about adding a |
@mickael-menu OK, that works for me. I've done so in the latest commit, let me know if this is what you were thinking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect thank you. If you could just remove the Properties.Add
mutable APIs and that's ready to merge for me. 🙏
@mickael-menu I've removed all the mutex code from the properties, and reverted it to simply being a |
This addresses crashes in the go-toolkit when used in highly concurrent access to publications, due to simultaneous updates to a link's properties (which is a
map[string]interface{}
) in other parts of the go-toolkit such as the archive fetcher.Unfortunately this ends up making the code more ugly due to the addition of a mutex (and thus conversion of the map to a struct with a map and mutex) as well a custom JSON marshalling needed to omit empty properties due to current limitations of go's stdlib json marshaller